Fix icpr_cert module error handling with missing HRESULT#21663
Open
zeroSteiner wants to merge 2 commits into
Open
Fix icpr_cert module error handling with missing HRESULT#21663zeroSteiner wants to merge 2 commits into
zeroSteiner wants to merge 2 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the MS-ICPR certificate request error handling to avoid crashes when an HRESULT lookup fails, and updates the windows_error dependency to a version/commit that improves Win32-facility HRESULT decoding.
Changes:
- Update the MS-ICPR error path to raise a safe “unknown HRESULT” message when the lookup fails.
- Switch
windows_errorto a git-sourced version that includes improved Win32 HRESULT lookups. - Update
Gemfile.lockto reflect the newwindows_errorsource/version.
Impact Analysis:
- Blast radius: medium — affects the MS-ICPR helper used by the
auxiliary/admin/dcerpc/icpr_certmodule (and any other callers ofMsf::Exploit::Remote::MsIcpr); also affects bundle resolution for anyone installing from this repo. - Data and contract effects: low — no schema/payload changes; behavior changes only in error handling/messages and dependency source.
- Rollback and test focus: rollback is straightforward (revert file + dependency changes); focus testing on certificate request failure paths where
response[:disposition]is present but unmapped, and ensure exceptions render actionable messages.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| lib/msf/core/exploit/remote/ms_icpr.rb | Adjusts unknown HRESULT handling to avoid crashing when lookup fails and to surface a hex code. |
| Gemfile | Adds windows_error as a git dependency to pick up improved HRESULT decoding behavior. |
| Gemfile.lock | Locks windows_error to the specified git revision/version and updates dependencies accordingly. |
Comment on lines
+129
to
+130
| if response[:disposition] | ||
| message = "Unknown HRESULT 0x#{response[:disposition].to_s(16).rjust(8, '0')}" |
Comment on lines
+129
to
+135
| if response[:disposition] | ||
| message = "Unknown HRESULT 0x#{response[:disposition].to_s(16).rjust(8, '0')}" | ||
| else | ||
| message = "Unknown error, no disposition code" | ||
| end | ||
|
|
||
| raise MsIcprUnknownError.new(message) |
| gem 'webmock', '~> 3.18' | ||
| end | ||
|
|
||
| gem 'windows_error', git: 'https://github.com/zeroSteiner/windows_error', branch: 'feat/win32-hresults' |
zeroSteiner
force-pushed
the
fix/icpr-missing-hresult
branch
from
6f623d1 to
d139f26
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
A crash was privately reported to me where the icpr_cert module was failing due to the
hresultbeingnilbecuase the constant didn't exist. When the reporter shared it with me, it was a win32 error with the win32 facility. The changes in this PR handle future, hypothetical scenarios where thehresultlookup fails so you at least get the hex value and can Google (ChatGPT?) it.There's also a gemfile bump that pulls in changes to the windows_error gem which adds lookups for the win32 error code when the facility is win32. These changes are in rapid7/windows_error#9
Related Issue: N/A this was privately reported to me
Breaking Changes
None
Reviewer Notes
Verification Steps
I wasn't able to reproduce the exact conditions where the
hresultwould be nil but there's not much to the logic here. It's pretty straight forward to see that.find_by_retvalcan returnniland then that accessing the message on it would cause a crash.Test Evidence
Environment
AI Usage Disclosure
I only used AI to check my work and only on the rapid7/windows_error PR.
Pre-Submission Checklist
documentation/modules(new modules only)lib/changes)Hardware and Complex Software Module Guidance
If your module targets specialized hardware (routers, IoT, PLCs, etc.) or complex software (licensed, multi-service, or multi-version), provide a pcap, screen recording, or video showing successful execution.
Email sanitized pcaps/recordings to msfdev@metasploit.com — remove real IPs, credentials, and hostnames before sending. If hardware/software is unavailable, explain in the PR description.
Responsiveness and PR Takeover Policy
We want every contribution to make it into the project. If approximately 2 weeks pass after a review request without a comment or code update from you, the team may take over the PR and complete the work on your behalf.
If this happens, you will remain credited as a co-author on the final commit — your contribution is always recognized.
This policy exists to keep the project moving forward. It is not a reflection on the quality of your work or your involvement. Life happens, and we would rather finish the work together than let a good contribution go stale.